Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add uploadthing driver #390

Merged
merged 17 commits into from
Dec 18, 2024
Merged

feat: add uploadthing driver #390

merged 17 commits into from
Dec 18, 2024

Conversation

juliusmarminge
Copy link
Contributor

@juliusmarminge juliusmarminge commented Jan 25, 2024

πŸ”— Linked issue

#389

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

src/drivers/uploadthing.ts Outdated Show resolved Hide resolved
src/drivers/uploadthing.ts Outdated Show resolved Hide resolved
src/drivers/uploadthing.ts Outdated Show resolved Hide resolved
src/drivers/uploadthing.ts Outdated Show resolved Hide resolved
src/drivers/uploadthing.ts Outdated Show resolved Hide resolved
@juliusmarminge
Copy link
Contributor Author

juliusmarminge commented Jan 25, 2024

@pi0 just tried to test this out and we stop usage of the utapi from client (mostly cause they shouldn't leak their api keys there)
CleanShot 2024-01-25 at 18 17 14

got any other good way to test this?


oh right and our api also doesn't enable cors so it cannot be used from browser anyways :)

CleanShot 2024-01-25 at 18 19 52

@pi0
Copy link
Member

pi0 commented Jan 25, 2024

We have unit tests you can run with pnpm run vitest (unstoage also targets server). We mainly need to mock endpoints (responding from mobile if couldn’t find it please tell me to push commit)

@juliusmarminge
Copy link
Contributor Author

I just realized that in order to support this we'd need to allow setting the key manually. right now uploadthing generates the key for you

@juliusmarminge
Copy link
Contributor Author

I just realized that in order to support this we'd need to allow setting the key manually. right now uploadthing generates the key for you

@pi0 is there a concept of mapping keys inside unstorage? For example when we setItem we get the key from UT, that we could then map to the unstorage key, i.e:

storage.setItem("foo")
// internally map foo -> KEY_FROM_UT
storage.getItem("foo")
// key = map["foo"], fetch(utfs.io/f/key)

@pi0
Copy link
Member

pi0 commented Jan 25, 2024

I think we mainly need a storage for keys which is usually same storage layer.

If your API is designed to always return random names, I am up to consider a new change that setItemRaw (setItem is mainly for text values) returns the random name. It is a non breaking change and i hope to unblock this presets also any other preset that need this.

(in the meantime, i guess it would be also nice if you consider supporting custom names. let's see which comes first haha)

@markflorkowski
Copy link

(in the meantime, I guess it would be also nice if you consider supporting custom names. let's see which comes first haha)

The reason we don't is because names must be URL-safe, and have a length limit.

@pi0
Copy link
Member

pi0 commented Jan 25, 2024

Hmm, I see. What if we make the driver make sure that input is url-safe and respects the limit like any fs does (so it throws an error if it doesn't comply) I think users of the driver, would understandably follow this and probably can benefit more from such control vs random ids. Thoughts?

@juliusmarminge
Copy link
Contributor Author

So I did a thing to track internal vs uploadthing keys - not sure if it aligns with what this project aims to do though - I'll let you be the judge.

Currently passing all tests except clear since our API throws a 500 when you try to delete 0 files currently (will fix that momentarily)

@markflorkowski
Copy link

Currently passing all tests except clear since our API throws a 500 when you try to delete 0 files currently (will fix that momentarily)

Merged fix for this on the API side

@juliusmarminge
Copy link
Contributor Author

current approach only works for a long-running server since it depends on that in-memory map. we're thinking about how to support user-provided keys

@juliusmarminge
Copy link
Contributor Author

CleanShot 2024-01-25 at 23 03 48

Now just gonna have to get the mocking right πŸ˜…

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: Patch coverage is 30.86420% with 56 lines in your changes missing coverage. Please review.

Project coverage is 58.70%. Comparing base (4d61c78) to head (e6a34fb).
Report is 129 commits behind head on main.

Files with missing lines Patch % Lines
src/drivers/uploadthing.ts 30.00% 56 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
- Coverage   65.05%   58.70%   -6.36%     
==========================================
  Files          39       43       +4     
  Lines        4055     3666     -389     
  Branches      487      582      +95     
==========================================
- Hits         2638     2152     -486     
- Misses       1408     1510     +102     
+ Partials        9        4       -5     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@juliusmarminge
Copy link
Contributor Author

struggling getting msw to intercept the requests πŸ€”

CleanShot 2024-01-25 at 23 15 52

@pi0
Copy link
Member

pi0 commented Jan 25, 2024

yeah. msw is not the best in class nor supports ofetch which I am aware of and also the fix but still hesitated because by doing it we add an overhead to every user of it only because of msw.

I will try to check it ASAP.

Copy link

nuxt-studio bot commented Jan 25, 2024

βœ… Live Preview ready!

Name Edit Preview Latest Commit
unstorage Edit on Studio β†—οΈŽ View Live Preview 4d55f54

@juliusmarminge
Copy link
Contributor Author

juliusmarminge commented Jan 30, 2024

@pi0 updated to use native custom ids now.

there are some tests failing however that's expected, since they relate to deletion of files.

UT is not a KV store, so when a file is deleted in UT, it is not instantly deleted but it may take up to 30 seconds before it is deleted at the storage provider, and then deleted from our db. some of the tests here assume that calling hasItem(key) directly after deleteItem(key) should return false, which is not the case for uploadthing.

idk how you wanna pursue given that knowledge. alter the tests? custom test suite for UT?


cc @markflorkowski we could, delete the customId field when a file is marked for deletion, so that a new file may be uploaded with that same customId even before the file is removed. idk if that would make sense for us to do, but it would be a workaround to fix this issue

another thing that we could do is to filter out the files marked for deletion in the getFileUrl endpoint. this would be a breaking change to the API but I don't see why they shouldn't be filtered out in the first place...

src/storage.ts Outdated Show resolved Hide resolved
@pi0
Copy link
Member

pi0 commented Jan 30, 2024

Exciting updates πŸš€ Sorry i'm quite busy still will try to come to this PR as soon as could also for failing tests.

Copy link
Contributor Author

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: bump deps once properly released

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@pi0 pi0 added the driver label May 1, 2024
@pi0
Copy link
Member

pi0 commented Dec 15, 2024

Hi dear @juliusmarminge and @markflorkowski

Finally back on this. I've updated client to v7 impl and made more reactors with base key handling, etc. customId is a really interesting idea and things almost work now!

Tests updated to run against a real (sandbox) endpoint. you need to update .env and manually run pnpm vitest test/drivers/uploadthing.test.ts (GH actions CI is ready and works after merge).


I found an interesting issue that seems happening in prod when listing keys. When a file with custom id is created and then deleted, any attempt to recreate it, it won't be returned from list anymore (!) ... which breaks getKeys test and initial test also if we remove random prefix in test file, might need you attention.

When that fixed, one last step is that we default to private ACL as this is the main purpose of unstorage for private server data.

@juliusmarminge
Copy link
Contributor Author

I tihnk what you're running into is this: pingdotgg/uploadthing#948

@pi0
Copy link
Member

pi0 commented Dec 18, 2024

Thinking of moving forward and iterating.

  • Added note in docs that driver is experimental
  • Added note in the docs about the known issue of same-key
  • Used a "cloud-like" icon for docs until there is an icon in iconify for uploadthing
  • Tests skipped in CI since getKeys is failing

@juliusmarminge
Copy link
Contributor Author

That sounds good to me. I'll try to remember and come back here when we've removed that limitation

@pi0 pi0 merged commit e049ce6 into unjs:main Dec 18, 2024
2 of 4 checks passed
@pi0 pi0 mentioned this pull request Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants